-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
server: refactor process*Payload functions #644
server: refactor process*Payload functions #644
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the spirit of this PR. Removing the write lock is nice. Channels are useful. I think it would be good to add back the deleted cancellation check, see my comment for more details.
server/service.go
Outdated
requestCtxCancel() | ||
*result = *responsePayload | ||
resultCh <- responsePayload | ||
log.Info("received payload from relay") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there could be value in adding back a cancellation check here. There's also a check at the beginning of the function, but this one (closer to the end) is most likely to catch it, right? This would help prevent multiple, relatively verbose "received payload from relay" messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added back a check that should get rid of multiple received payload from relay messages, but its kinda ugly. I kinda prefer the multiple log lines, ptal
server/service.go
Outdated
wg.Wait() | ||
close(resultCh) | ||
result := <-resultCh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a strange pattern, but it should work as intended. We still need the wait group unfortunately. Normally, reading from a channel is a blocking operation, but if the channel is closed it's not blocking. It would be cool if we could figure out a way of handling this better, where we don't necessarily have to wait for each request to finish. As in, remove the wait group and proceed as soon as we receive the first request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the wg and changed the pattern so that we only need one request to finish / or the timeout to trigger
Somethings wrong... am investigating |
d169095
to
ebb5790
Compare
Seems like the issue was the same as in #650 |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #644 +/- ##
===========================================
- Coverage 43.84% 43.69% -0.16%
===========================================
Files 15 15
Lines 1608 1609 +1
===========================================
- Hits 705 703 -2
- Misses 847 849 +2
- Partials 56 57 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
16c8e04
to
4f970ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice refactor and simplification, thanks 👍
This PR changes the way the process functions work. Since it is safe to call
cancel()
multiple times, and we only need one valid payload from the relays, I think its safe to stick the results in a channel and only query one of them